Skip to content

Avoid Lua full-set reads for unfiltered function lists - #13

Merged
rdmmf merged 1 commit into
MISP:devfrom
SegmondFault:fix/function-search-partial-index-hang
Jul 27, 2026
Merged

Avoid Lua full-set reads for unfiltered function lists#13
rdmmf merged 1 commit into
MISP:devfrom
SegmondFault:fix/function-search-partial-index-hang

Conversation

@SegmondFault

@SegmondFault SegmondFault commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Avoid routing unfiltered function list requests through the Lua search path.

When the Functions table loads without filters, the existing Lua script can fall back to reading collection:all_functions before pagination. In a large or partially ingested collection, this can make /api/function/search?collection=main&limit=100&min_cohesion=0.95 time out and leave Kvrocks unresponsive.

This adds a bounded route-level path for unfiltered function list requests:

  • uses SCARD for the total count
  • uses an existing sorted function index when available
  • falls back to bounded SSCAN for the requested page
  • keeps filtered searches on the existing Lua producer/filter path

Addresses #12

Provenance

This PR came from reproducing a Kvrocks hang while investigating the function-search dashboard path on a large/partially indexed main collection. The original symptom was an unfiltered function-list request timing out, followed by Kvrocks becoming unresponsive to PING.

The initial PR was opened against main, but after maintainer feedback that current stable work is on dev, I fetched MISP/bsimvis dev and rechecked the change there.

On current origin/dev, the broader build-sim Kvrocks EVAL-lock issue appears to have been addressed separately by:

cd95b34 perf(sim): reimplement build-sim discovery in Python to drop kvrocks EVAL lock

PR #13 is therefore scoped more narrowly: it still covers the unfiltered function-search/listing path. On origin/dev, that path still calls search_function.lua with an empty groups list, and the Lua script still falls back to:

raw_ids = redis.call('SMEMBERS', producer.key)

where producer.key is collection .. ":all_functions".

Validation Against dev

I created a temporary detached worktree from origin/dev, cherry-picked this PR commit onto it, and verified that it applies cleanly.

Validation performed after retargeting the PR to dev:

  • git diff --check origin/dev..HEAD
  • uv run python -m compileall bsimvis/app/routes/search_function.py
  • GitHub reports the retargeted PR as mergeable

I also ran a small fake-Redis route harness to verify the failure trigger without stressing a real Kvrocks instance:

  • unpatched origin/dev: GET /api/function/search?collection=main&limit=2 reaches Lua with groups: [], reproducing the risky SMEMBERS main:all_functions path
  • patched PR branch on top of dev: the same request does not call Lua; it uses SCARD main:all_functions, bounded SSCAN main:all_functions, then the normal enrichment pipeline

Implementation Notes

I initially tried keeping this entirely inside bsimvis/app/lua/search_function.lua by replacing the unfiltered SMEMBERS path with bounded Lua-side pagination. That still reproduced the failure in the isolated repro runtime: the request timed out after about 20s and Kvrocks stopped responding to PING.

Because of that, this PR avoids invoking the Lua search script at all for the no-filter function list case. Filtered searches still use the existing Lua producer/filter path.

Scope

This is intentionally limited to bsimvis/app/routes/search_function.py and the default unfiltered function list path. It does not change the UI, storage schema, ingestion/indexing model, similarity build logic, or filtered search behavior.

Tradeoffs

This PR intentionally avoids adding a new ordered function-list index, so there are two remaining edge cases in the fallback path:

  1. Deep pagination without a sort index may still be slower.
    If the request asks for a high offset and there is no usable sorted function index, the fallback has to scan past earlier set members before collecting the requested page. This is still bounded to page collection rather than loading the full function set into Lua, but it is not as efficient as a real ordered index.

  2. Default sort_by=id fallback is not globally sorted.
    collection:all_functions is a set, so SSCAN does not provide stable global ordering. The fallback returns a bounded page of function IDs, but it does not reproduce the old behavior of loading and sorting the entire set by ID. Preserving stable ID ordering efficiently would require a maintained ordered function-list index.

A future follow-up could add/backfill such an index, but that would touch ingestion/indexing behavior, so I left it out to keep this PR focused on the reproduced Kvrocks hang.

Testing

  • luac -p bsimvis/app/lua/search_function.lua
  • PYTHONPYCACHEPREFIX=/tmp/bsimvis-pycache uv run python -m py_compile bsimvis/app/routes/search_function.py
  • git diff --check origin/main...HEAD
  • Verified against the original isolated repro runtime:
    • before fix: request timed out after about 20s and Kvrocks PING hung
    • Lua-side pagination attempt: still timed out after about 20s and Kvrocks PING hung
    • after this route-level fix: request returned HTTP 200 total 0.014008
    • post-request Kvrocks PING returned PONG
  • Revalidated after retargeting to dev:
    • cherry-pick onto temporary origin/dev worktree applied cleanly
    • git diff --check origin/dev..HEAD
    • uv run python -m compileall bsimvis/app/routes/search_function.py
    • fake-Redis harness confirmed unpatched dev reaches the unfiltered Lua path and patched dev avoids it

@SegmondFault
SegmondFault changed the base branch from main to dev July 14, 2026 19:16
rdmmf added a commit that referenced this pull request Jul 27, 2026
The unfiltered function-list path added in #13 paged straight off SSCAN,
which has no stable ordering. Consecutive pages could overlap or skip
rows, and the same request could return a different page each time — the
old Lua path sorted by id ascending.

Use the same shape as the unfiltered paths in search_file and
search_feature: load the set, sort in Python, slice. The hang in #12 came
from doing this inside Lua under the Kvrocks global EVAL lock, which #13
already removed; from Python the sibling routes have done it this way all
along.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@rdmmf
rdmmf merged commit b86098d into MISP:dev Jul 27, 2026
rdmmf added a commit that referenced this pull request Jul 27, 2026
* Avoid Lua full-set reads for unfiltered function lists

* fix(search): keep unfiltered function pages in stable id order

The unfiltered function-list path added in #13 paged straight off SSCAN,
which has no stable ordering. Consecutive pages could overlap or skip
rows, and the same request could return a different page each time —
the old Lua path sorted by id ascending.

There is no id-ordered index to range over (save_function only does
SADD on {col}:all_functions), so bound the scan instead: collect up to
SORT_SCAN_CAP members, sort by id, slice the page, and report anything
beyond the cap via the existing pool_truncated flag. The scan stays
incremental, so it never holds Kvrocks the way the SMEMBERS-in-Lua path
did.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(search): keep unfiltered function pages in stable id order

The unfiltered function-list path added in #13 paged straight off SSCAN,
which has no stable ordering. Consecutive pages could overlap or skip
rows, and the same request could return a different page each time — the
old Lua path sorted by id ascending.

Use the same shape as the unfiltered paths in search_file and
search_feature: load the set, sort in Python, slice. The hang in #12 came
from doing this inside Lua under the Kvrocks global EVAL lock, which #13
already removed; from Python the sibling routes have done it this way all
along.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: SegmondFault <izaakalfredgray@gmail.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
rdmmf added a commit that referenced this pull request Jul 27, 2026
* Avoid Lua full-set reads for unfiltered function lists

* fix(search): keep unfiltered function pages in stable id order

The unfiltered function-list path added in #13 paged straight off SSCAN,
which has no stable ordering. Consecutive pages could overlap or skip
rows, and the same request could return a different page each time —
the old Lua path sorted by id ascending.

There is no id-ordered index to range over (save_function only does
SADD on {col}:all_functions), so bound the scan instead: collect up to
SORT_SCAN_CAP members, sort by id, slice the page, and report anything
beyond the cap via the existing pool_truncated flag. The scan stays
incremental, so it never holds Kvrocks the way the SMEMBERS-in-Lua path
did.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(search): keep unfiltered function pages in stable id order

The unfiltered function-list path added in #13 paged straight off SSCAN,
which has no stable ordering. Consecutive pages could overlap or skip
rows, and the same request could return a different page each time — the
old Lua path sorted by id ascending.

Use the same shape as the unfiltered paths in search_file and
search_feature: load the set, sort in Python, slice. The hang in #12 came
from doing this inside Lua under the Kvrocks global EVAL lock, which #13
already removed; from Python the sibling routes have done it this way all
along.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: SegmondFault <izaakalfredgray@gmail.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants